Skip to content

Add readBinaryWithFeatures to JS API#8541

Merged
tlively merged 2 commits intoWebAssembly:mainfrom
ospencer:oscar/add-read-module-features-js
Mar 30, 2026
Merged

Add readBinaryWithFeatures to JS API#8541
tlively merged 2 commits intoWebAssembly:mainfrom
ospencer:oscar/add-read-module-features-js

Conversation

@ospencer
Copy link
Copy Markdown
Contributor

Realized this was missing when I tried to reach for it.

@ospencer ospencer requested a review from a team as a code owner March 28, 2026 21:49
@ospencer ospencer requested review from tlively and removed request for a team March 28, 2026 21:49
binaryen.setDebugInfo(false);
module.dispose();

module = binaryen.readBinaryWithFeatures(buffer, features);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to test the failing case here where we don't have the features?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

Copy link
Copy Markdown
Contributor

@spotandjake spotandjake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

Copy link
Copy Markdown
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@tlively tlively merged commit 1a8150c into WebAssembly:main Mar 30, 2026
16 checks passed
@kripken
Copy link
Copy Markdown
Member

kripken commented Mar 30, 2026

If it's not too late (I see this is merged), another option might be to add this to the existing readBinary(), that is, add a second param on line 3308? If the param is undefined, we can set it to MVP features.

I guess the larger question is, do we want to mirror the C API, or do something more JS-ey with optional parameters. We do already use the check-for-undefined pattern elsewhere, so I lean towards that, but don't feel strongly.

@tlively
Copy link
Copy Markdown
Member

tlively commented Mar 30, 2026

Oh yeah, that's a good idea 😅

@kripken
Copy link
Copy Markdown
Member

kripken commented Mar 31, 2026

Does that sound reasonable to you too @ospencer @spotandjake ? If so I can open a PR.

@ospencer
Copy link
Copy Markdown
Contributor Author

I don't think it's a bad idea, but it might be a little confusing, maybe. I'd expect readBinary with no parameters to include all features by default and then let you restrict them if you wanted, not the other way around. But in the context of knowing how readBinary already works, it's probably fine.

@kripken
Copy link
Copy Markdown
Member

kripken commented Mar 31, 2026

Hmm, yeah, good point, enabling all features would be more natural here. But we use MVP features in the original version in the C API for backwards compatibility, so the C and JS APIs would mismatch if we did that. Maybe it's not worth changing anything, given that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants